Conversation
azasypkin
added
release highlight
Team:Security
* Add user profile UI * add code doc * More * Fix nav control for anonymous users * Move presentational logic outside form row * Fix disabled state and update profile api * remove presentational logic from form row * wip * Fix initials * . * empty user avatar * simplify reserved user screen * . * unit tests * type fixes * . * . * . * . * . * . * . * . * . * . * . * . * . * . * . * . * . * [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix' * . * . * . * . * . * . * Review#1: handle UI inconsistencies, update avatar in the navigation bar when profile data is updated, etc. * Fix type errors. * Use different validation model for the first and subsequent submits. * Review#2: move change password form to a modal, support users authenticated via authentication proxies. * Use proper test subject for the password change Cancel button. * Review#3: retry profile activation requests. * Review#4: align text color of the non-editable profile fields with the designs. * Review#4: improve warning message for the change password form when changing password for Kibana system users. Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Aleh Zasypkin <aleh.zasypkin@elastic.co>
azasypkin
added
ci:deploy-cloud
and removed
ci:no-auto-commit
|
Okay, CI is finally green and we have a functioning Cloud deployment 🎉 |
MichaelMarcialis
left a comment
MichaelMarcialis
left a comment
There was a problem hiding this comment.
Thanks for making those changes, @azasypkin! Leaving you two final comments for the re-review:
Disabled text color
Changed to
$euiColorDisabled(I assume you meant this one and not$euiColorDisabledText) in 92988cd
They're actually the same hex values, but I did indeed intend to use $euiColorDisabledText (or euiTheme.colors.disabledText), just in case the EUI team changes the value specifically for the text version in the future. Can we change that?
Page template padding
With the swapping that happened between KibanaPageTemplate and EuiPageTemplate, I think the padding got messed up again in the process. It looks like there is no padding being applied now, apparent when viewing at small viewport sizes.
I'm guessing this is happening because there is a paddingSize="none" prop being applied to the EuiPageTemplate. If so, can you remove that prop (which should theoretically apply the default paddingSize="l")?
Otherwise, this looks good to me. I'm going to go ahead and approve it now so I don't hold you up any further, under the assumption that you can address the above comments. Thanks so much!
Hmm, when I use |
jportner
left a comment
jportner
left a comment
There was a problem hiding this comment.
I'm not sure if you intended to address #132645 before merging this to main or not, but I just wanted to remind you of it.
If you want to follow up afterwards, that's fine.
I commented with a few minor nits from that issue and a few new ones that I found. Take them or leave them 😄 LGTM
x-pack/plugins/security/public/account_management/user_profile/user_profile.tsx
Outdated
Show resolved
Hide resolved
| const canvas = document.createElement('canvas'); | ||
| const context = canvas.getContext('2d'); | ||
| if (context) { | ||
| if (image.width >= image.height) { | ||
| canvas.width = maxSize; | ||
| canvas.height = Math.floor((image.height * maxSize) / image.width); | ||
| } else { | ||
| canvas.height = maxSize; | ||
| canvas.width = Math.floor((image.width * maxSize) / image.height); | ||
| } | ||
| context.drawImage(image, 0, 0, canvas.width, canvas.height); | ||
| const resizedDataUrl = canvas.toDataURL(); |
There was a problem hiding this comment.
This bit of code (0, 0) always draws the image from the top left corner.
So, if for example you had an image that was much taller than it was wide, uploading it as your avatar would only show the topmost part of the image.
It bugged me a bit so I figured out how to make it use the appropriate offset to center an image:
diff --git a/x-pack/plugins/security/public/account_management/user_profile/utils.ts b/x-pack/plugins/security/public/account_management/user_profile/utils.ts
index fd153502884..3ef60ad585e 100644
--- a/x-pack/plugins/security/public/account_management/user_profile/utils.ts
+++ b/x-pack/plugins/security/public/account_management/user_profile/utils.ts
@@ -28,15 +28,19 @@ export function resizeImage(imageUrl: string, maxSize: number) {
try {
const canvas = document.createElement('canvas');
const context = canvas.getContext('2d');
+ let xOffset = 0;
+ let yOffset = 0;
if (context) {
if (image.width >= image.height) {
canvas.width = maxSize;
canvas.height = Math.floor((image.height * maxSize) / image.width);
+ xOffset = Math.floor((canvas.height - canvas.width) / 2);
} else {
canvas.height = maxSize;
canvas.width = Math.floor((image.width * maxSize) / image.height);
+ yOffset = Math.floor((canvas.width - canvas.height) / 2);
}
- context.drawImage(image, 0, 0, canvas.width, canvas.height);
+ context.drawImage(image, xOffset, yOffset, canvas.width, canvas.height);
const resizedDataUrl = canvas.toDataURL();
return resolve(resizedDataUrl);
}There was a problem hiding this comment.
Funnily enough, centering "tall" images works much worse for the image set I have since the head in tall photos is usually close to (0,0) and centering clips the head sometimes like in the picture below:
I'll keep as is for now, and we can revisit it with @thomheymann once he's back
x-pack/plugins/security/public/components/api_clients_provider.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security/public/account_management/user_profile/user_avatar.tsx
Outdated
Show resolved
Hide resolved
| export function useUserProfile<T extends UserData>(dataPath?: string) { | ||
| const { userProfiles } = useApiClients(); | ||
| const dataUpdateState = useObservable(userProfiles.dataUpdates$); | ||
| return useAsync(() => userProfiles.get<T>(dataPath), [userProfiles, dataUpdateState]); | ||
| } |
There was a problem hiding this comment.
Reminder about my prior feedback: #127624 (comment)
This is tracked as part of #132645. Since we are no longer shipping this enhancement with 8.3, I think we should take the time to go ahead and rename this to something else like useMyUserProfile
x-pack/plugins/security/server/authentication/authentication_result.ts
Outdated
Show resolved
Hide resolved
Ah, you're right. I was looking at the old Sass variables. Some of the newer Emotion colors are calculated slightly differently than they were previously and we still need to update the design system in Figma to get them back in sync. Thanks for catching this. In any case, let's go ahead and proceed with |
Roger that, switched to |
💛 Build succeeded, but was flakyFailed CI StepsTest Failures
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
async chunk count
ESLint disabled line counts
References to deprecated APIs
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
tylersmalley
added
ci:cloud-deploy
11 participants
Summary
This PR merges
feature/user-profilebranch intomainbranch.Blocked by: #132645Release note: Users can have their personal avatars now.